Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DXCDT-378: Keyword Replacement E2E Tests #735

Merged
merged 9 commits into from
Feb 8, 2023

Conversation

willvedd
Copy link
Contributor

@willvedd willvedd commented Feb 8, 2023

🔧 Changes

As we embark on work to address #328 , it is important to not introduce any regressions with the current keyword replacement functionality. While this functionality has a decent amount of unit-testing, the coverage in our end-to-end tests are lacking. This PR introduces a baseline of testing to fill this gap.

Notably, this PR found a bug that prevents the usage of the @@ array replacement syntax with the directory format (pointed out in comment below). Which just further highlights the need for this testing. Update: This was actually a mistake in the implementation of the E2E test and has been corrected in a separate commit!

🔬 Testing

This PR only introduces testing additions.

📝 Checklist

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

@willvedd willvedd requested a review from a team as a code owner February 8, 2023 17:38
@willvedd willvedd changed the title Dxcdt 378 e2e test keyword replacement DXCDT-378: Keyword Replacement E2E Tests Feb 8, 2023

const keywordMapping = {
COMPANY_NAME: 'Travel0',
//LANGUAGES: ['en', 'es'], //TODO: support array replacement for directory format
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was discovered during this work that the @@ array syntax does not work for directory formats. It does work for the YAML handler though. This will need to be addressed in a follow-up PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Actually, this is a mistake with how I did the E2E test originally and has since been fixed.

Copy link
Contributor

@sergiught sergiught left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work here 💪🏻

Just left suggestions to add new empty lines at the end of each test data file.

@codecov-commenter
Copy link

Codecov Report

Base: 83.60% // Head: 83.60% // No change to project coverage 👍

Coverage data is based on head (10f7a26) compared to base (0221678).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #735   +/-   ##
=======================================
  Coverage   83.60%   83.60%           
=======================================
  Files         114      114           
  Lines        3379     3379           
  Branches      628      628           
=======================================
  Hits         2825     2825           
  Misses        324      324           
  Partials      230      230           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@willvedd willvedd enabled auto-merge (squash) February 8, 2023 21:19
@willvedd willvedd merged commit eb38eff into master Feb 8, 2023
@willvedd willvedd deleted the DXCDT-378-e2e-test-keyword-replacement branch February 8, 2023 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants